Skip to content

Bug fixed. Dependency inverted. Pre-existing flaky test fixed#845

Open
Ni7ram wants to merge 3 commits into
splitio:developmentfrom
Ni7ram:Fix-FactoryPatch
Open

Bug fixed. Dependency inverted. Pre-existing flaky test fixed#845
Ni7ram wants to merge 3 commits into
splitio:developmentfrom
Ni7ram:Fix-FactoryPatch

Conversation

@Ni7ram
Copy link
Copy Markdown

@Ni7ram Ni7ram commented May 20, 2026

Problem

Users who held only a reference to SplitClient (releasing the SplitFactory) could
experience the SDK never reaching .sdkReady. The factory was the sole strong owner of
SplitClientManager, which in turn owned eventsManagerCoordinator, syncManager and
all the infrastructure required to drive the client to ready. Once the factory was
released, everything died and the client became inert.

A workaround existed in SplitEventActionTask, which kept a strong factory: SplitFactory
property purely to keep the factory alive via the cycle
factory → clientManager → eventsManagerCoordinator → eventsManager → task → factory.
This worked only as long as the user registered at least one event listener and coupled
an event-running task to factory ownership.

Changes

This PR replaces the workaround with a direct ownership change:

  • DefaultSplitClient.clientManager: weak → strong (let). The client now keeps its
    client manager (and the rest of the infra) alive on its own.
  • SplitEventActionTask.factory removed (and from all call sites).
  • SplitClientManager.splitFactory removed (only consumer was the task workaround).
  • LocalhostSplitClient.clientManager removed (was unused).
  • All mocks/stubs and tests updated accordingly.

Existing cycles between DefaultClientManager ↔ defaultClient and through
byKeyRegistry are preserved by design; they get broken by destroy() exactly as
before, so no leak semantics change. The difference is that the cycle now reflects the
real ownership model rather than an opaque workaround.

Bonus

Fixes a pre-existing flaky test in LocalhostTests.testUsingYamlFromApi: the
.sdkUpdated listener was being registered after updateLocalhost(yaml:) was called,
which meant the event could fire before the listener was attached and be silently
dropped by DefaultSplitClient.on(event:)'s eventAlreadyTriggered guard. Reordered to
register first, trigger second.

@Ni7ram Ni7ram requested a review from a team as a code owner May 20, 2026 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants